Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

editoast: refactor infra tests part 7 #7932

Merged
merged 8 commits into from
Jul 10, 2024
Merged

editoast: refactor infra tests part 7 #7932

merged 8 commits into from
Jul 10, 2024

Conversation

hamz2a
Copy link
Contributor

@hamz2a hamz2a commented Jul 3, 2024 •
edited
Loading

Part of #6980

It's better to review the PR commit by commit.
I created intermediate functions {function}_v2 to facilitate the migration, but I deleted everything at the end.

I also refactored persist_railjson to use DbConnection directly instead of DbConnectionPool. Consequently, I removed the macro persist! that was in there and futures::try_join!, and I added a transaction.

@hamz2a hamz2a requested a review from a team as a code owner July 3, 2024 11:14
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2024 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 82.45614% with 30 lines in your changes missing coverage. Please review.

Project coverage is 28.21%. Comparing base (bea5cee) to head (6b5fd5a).
Report is 7 commits behind head on dev.

Files Patch % Lines
editoast/src/views/infra/edition.rs 59.37% 13 Missing ⚠️
editoast/src/views/infra/mod.rs 66.66% 7 Missing ⚠️
editoast/src/views/infra/railjson.rs 54.54% 5 Missing ⚠️
editoast/src/views/infra/routes.rs 57.14% 3 Missing ⚠️
editoast/src/views/infra/auto_fixes/mod.rs 77.77% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@             Coverage Diff              @@
##                dev    #7932      +/-   ##
============================================
- Coverage     28.25%   28.21%   -0.05%     
  Complexity     2075     2075              
============================================
  Files          1276     1276              
  Lines        156193   156314     +121     
  Branches       3084     3084              
============================================
- Hits          44136    44102      -34     
- Misses       110216   110371     +155     
  Partials       1841     1841
Flag Coverage Δ
core 75.03% <ø> (ø)
editoast 70.79% <82.45%> (-0.23%) ⬇️
front 9.91% <ø> (-0.01%) ⬇️
gateway 2.34% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 72.93% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

woshilapin
woshilapin requested changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about why we do need a lot of #[serial_test::serial] in commit 3040c34.

Also, I'd like to be convinced that removing futures::try_join_all!() is fine. But to me, it should affect the time of execution for persist_railjson unless I'm missing something.

Apart from that, great work in separating commits in this PR. Thank you, it really eased the review for this big PR.

@hamz2a
Copy link
Contributor Author

hamz2a commented Jul 3, 2024

I'm curious about why we do need a lot of #[serial_test::serial] in commit 3040c34.

I added #[serial_test::serial] because I was getting deadlock errors.

@hamz2a hamz2a force-pushed the hai/editoast-refactor-infra-tests-part-7 branch from 9d078dc to 281e005 Compare July 3, 2024 16:20
@leovalais leovalais self-requested a review July 3, 2024 16:26
@hamz2a hamz2a force-pushed the hai/editoast-refactor-infra-tests-part-7 branch 3 times, most recently from fc3d06d to f8d2164 Compare July 3, 2024 17:15
@woshilapin
Copy link
Contributor

I'm curious about why we do need a lot of #[serial_test::serial] in commit 3040c34.

I added #[serial_test::serial] because I was getting deadlock errors.

We dug a little bit to try to understand. It's a deadlock detected by the Postgresql instance itself. It might be due to concurrence on the generated id for infras since we do create very long transactions when working with infras in tests.

We did not find an obvious solution to work around the problem. Let's keep it like this.

@hamz2a hamz2a force-pushed the hai/editoast-refactor-infra-tests-part-7 branch from f8d2164 to 96e803f Compare July 4, 2024 09:33
leovalais
leovalais requested changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this work! PoolV1 is almost gone! 😄

@hamz2a hamz2a force-pushed the hai/editoast-refactor-infra-tests-part-7 branch 2 times, most recently from c6d6900 to 4797493 Compare July 4, 2024 15:55
@hamz2a hamz2a requested a review from leovalais July 5, 2024 09:33
@hamz2a hamz2a force-pushed the hai/editoast-refactor-infra-tests-part-7 branch from f0012b4 to 584d0d3 Compare July 5, 2024 14:46
leovalais
leovalais requested changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! A few more and 🚀
I'll test your PR on Monday.

There are commits like merge branch dev or rebase in your history. Those will need to be squashed.

@hamz2a hamz2a force-pushed the hai/editoast-refactor-infra-tests-part-7 branch 3 times, most recently from a202f91 to ef3ef96 Compare July 9, 2024 08:23
@woshilapin woshilapin force-pushed the hai/editoast-refactor-infra-tests-part-7 branch from ef3ef96 to f0c0ee5 Compare July 9, 2024 21:02
@hamz2a hamz2a requested a review from leovalais July 10, 2024 08:32
@woshilapin woshilapin force-pushed the hai/editoast-refactor-infra-tests-part-7 branch 3 times, most recently from aea5510 to 5136146 Compare July 10, 2024 09:23
@hamz2a hamz2a force-pushed the hai/editoast-refactor-infra-tests-part-7 branch from 5136146 to 6b5fd5a Compare July 10, 2024 14:55
leovalais
leovalais approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job and thank you for the investigation of the deadlocks bug and for rearranging the history!

Once this is merged we'll need to check that all non-TSV1 endpoints are using DbPoolV2 and that we didn't miss any test that was using TestFixture.

After that we can probably work on making the DbConnection opaque to allow us to remove any diesel from views :)

@hamz2a hamz2a added this pull request to the merge queue Jul 10, 2024
Merged via the queue into dev with commit f49e67c Jul 10, 2024
17 checks passed
@hamz2a hamz2a deleted the hai/editoast-refactor-infra-tests-part-7 branch July 10, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woshilapin woshilapin woshilapin approved these changes

@leovalais leovalais leovalais approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hamz2a @codecov-commenter @woshilapin @leovalais